[Clang] Apply exclude_from_explicit_instantiation to dllimport/dllexport #168171
Conversation
Attaching `__declspec(dllexport/dllimport)` to explicit instantiation declaration made its whole member instantiated even if they were `__attribute__((__exclude_from_explicit_instantation__))`. Such members should not be instantiated nor be exported to avoid symbol leakage or duplication.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Tomohiro Kashiwada (kikairoya) ChangesAttaching Full diff: https://github.com/llvm/llvm-project/pull/168171.diff 3 Files Affected:
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index aa36a79142e52..fe732a1328fab 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -6551,6 +6551,8 @@ void Sema::checkClassLevelDLLAttribute(CXXRecordDecl *Class) {
return;
}
+ TemplateSpecializationKind TSK = Class->getTemplateSpecializationKind();
+
if (Context.getTargetInfo().shouldDLLImportComdatSymbols() &&
!ClassAttr->isInherited()) {
// Diagnose dll attributes on members of class with dll attribute.
@@ -6561,6 +6563,11 @@ void Sema::checkClassLevelDLLAttribute(CXXRecordDecl *Class) {
if (!MemberAttr || MemberAttr->isInherited() || Member->isInvalidDecl())
continue;
+ if ((TSK == TSK_ExplicitInstantiationDeclaration ||
+ TSK == TSK_ExplicitInstantiationDefinition) &&
+ Member->hasAttr<ExcludeFromExplicitInstantiationAttr>())
+ continue;
+
Diag(MemberAttr->getLocation(),
diag::err_attribute_dll_member_of_dll_class)
<< MemberAttr << ClassAttr;
@@ -6583,8 +6590,6 @@ void Sema::checkClassLevelDLLAttribute(CXXRecordDecl *Class) {
!ClassExported &&
cast<DLLImportAttr>(ClassAttr)->wasPropagatedToBaseTemplate();
- TemplateSpecializationKind TSK = Class->getTemplateSpecializationKind();
-
// Ignore explicit dllexport on explicit class template instantiation
// declarations, except in MinGW mode.
if (ClassExported && !ClassAttr->isInherited() &&
@@ -6601,6 +6606,11 @@ void Sema::checkClassLevelDLLAttribute(CXXRecordDecl *Class) {
// seem to be true in practice?
for (Decl *Member : Class->decls()) {
+ if ((TSK == TSK_ExplicitInstantiationDeclaration ||
+ TSK == TSK_ExplicitInstantiationDefinition) &&
+ Member->hasAttr<ExcludeFromExplicitInstantiationAttr>())
+ continue;
+
VarDecl *VD = dyn_cast<VarDecl>(Member);
CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(Member);
diff --git a/clang/test/CodeGenCXX/attr-exclude_from_explicit_instantiation.exclude_from_dllexport.cpp b/clang/test/CodeGenCXX/attr-exclude_from_explicit_instantiation.exclude_from_dllexport.cpp
new file mode 100644
index 0000000000000..0a94bd4b1f6b6
--- /dev/null
+++ b/clang/test/CodeGenCXX/attr-exclude_from_explicit_instantiation.exclude_from_dllexport.cpp
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 -triple x86_64-win32 -fms-extensions -emit-llvm -o - %s | FileCheck %s --check-prefixes=MSC --implicit-check-not=to_be_
+// RUN: %clang_cc1 -triple x86_64-mingw -emit-llvm -o - %s | FileCheck %s --check-prefixes=GNU --implicit-check-not=to_be_
+// RUN: %clang_cc1 -triple x86_64-cygwin -emit-llvm -o - %s | FileCheck %s --check-prefixes=GNU --implicit-check-not=to_be_
+
+// Test that __declspec(dllexport) doesn't instantiate entities marked with
+// the exclude_from_explicit_instantiation attribute unless marked as dllexport explicitly.
+
+#define EXCLUDE_FROM_EXPLICIT_INSTANTIATION __attribute__((exclude_from_explicit_instantiation))
+
+template <class T>
+struct C {
+ // This will be instantiated explicitly as an exported function because it
+ // inherits dllexport from the class instantiation.
+ void to_be_exported() noexcept;
+
+ // This will be instantiated implicitly as an exported function because it is
+ // marked as dllexport explicitly.
+ EXCLUDE_FROM_EXPLICIT_INSTANTIATION __declspec(dllexport) void to_be_exported_explicitly() noexcept;
+
+ // This will be instantiated implicitly but won't be exported.
+ EXCLUDE_FROM_EXPLICIT_INSTANTIATION void not_to_be_exported() noexcept;
+
+ // This won't be instantiated.
+ EXCLUDE_FROM_EXPLICIT_INSTANTIATION void not_to_be_instantiated() noexcept;
+};
+
+template <class T> void C<T>::to_be_exported() noexcept {}
+template <class T> void C<T>::to_be_exported_explicitly() noexcept {}
+template <class T> void C<T>::not_to_be_exported() noexcept {}
+template <class T> void C<T>::not_to_be_instantiated() noexcept {}
+
+// MSC: $"?to_be_exported@?$C@H@@QEAAXXZ" = comdat any
+// MSC: $"?to_be_exported_explicitly@?$C@H@@QEAAXXZ" = comdat any
+// MSC: $"?not_to_be_exported@?$C@H@@QEAAXXZ" = comdat any
+// GNU: $_ZN1CIiE14to_be_exportedEv = comdat any
+// GNU: $_ZN1CIiE25to_be_exported_explicitlyEv = comdat any
+// GNU: $_ZN1CIiE18not_to_be_exportedEv = comdat any
+
+// MSC: define weak_odr dso_local dllexport{{.*}} void @"?to_be_exported@?$C@H@@QEAAXXZ"
+// GNU: define weak_odr dso_local dllexport{{.*}} void @_ZN1CIiE14to_be_exportedEv
+template struct __declspec(dllexport) C<int>;
+
+void use() {
+ C<int> c;
+
+ // MSC: call void @"?to_be_exported_explicitly@?$C@H@@QEAAXXZ"
+ // GNU: call void @_ZN1CIiE25to_be_exported_explicitlyEv
+ c.to_be_exported_explicitly(); // implicitly instantiated here
+
+ // MSC: call void @"?not_to_be_exported@?$C@H@@QEAAXXZ"
+ // GNU: call void @_ZN1CIiE18not_to_be_exportedEv
+ c.not_to_be_exported(); // implicitly instantiated here
+};
+
+// MSC: define weak_odr dso_local dllexport{{.*}} void @"?to_be_exported_explicitly@?$C@H@@QEAAXXZ"
+// GNU: define weak_odr dso_local dllexport{{.*}} void @_ZN1CIiE25to_be_exported_explicitlyEv
+
+// MSC: define linkonce_odr dso_local void @"?not_to_be_exported@?$C@H@@QEAAXXZ"
+// GNU: define linkonce_odr dso_local void @_ZN1CIiE18not_to_be_exportedEv
diff --git a/clang/test/CodeGenCXX/attr-exclude_from_explicit_instantiation.exclude_from_dllimport.cpp b/clang/test/CodeGenCXX/attr-exclude_from_explicit_instantiation.exclude_from_dllimport.cpp
new file mode 100644
index 0000000000000..b070259c2f048
--- /dev/null
+++ b/clang/test/CodeGenCXX/attr-exclude_from_explicit_instantiation.exclude_from_dllimport.cpp
@@ -0,0 +1,58 @@
+// RUN: %clang_cc1 -triple x86_64-win32 -fms-extensions -emit-llvm -o - %s | FileCheck %s --check-prefixes=MSC --implicit-check-not=to_be_
+// RUN: %clang_cc1 -triple x86_64-mingw -emit-llvm -o - %s | FileCheck %s --check-prefixes=GNU --implicit-check-not=to_be_
+// RUN: %clang_cc1 -triple x86_64-cygwin -emit-llvm -o - %s | FileCheck %s --check-prefixes=GNU --implicit-check-not=to_be_
+
+// Test that __declspec(dllimport) doesn't instantiate entities marked with
+// the exclude_from_explicit_instantiation attribute unless marked as dllimport explicitly.
+
+#define EXCLUDE_FROM_EXPLICIT_INSTANTIATION __attribute__((exclude_from_explicit_instantiation))
+
+template <class T>
+struct C {
+ // This will be instantiated explicitly as an imported function because it
+ // inherits dllimport from the class instantiation.
+ void to_be_imported() noexcept;
+
+ // This will be instantiated implicitly as an imported function because it is
+ // marked as dllimport explicitly.
+ EXCLUDE_FROM_EXPLICIT_INSTANTIATION __declspec(dllimport) void to_be_imported_explicitly() noexcept;
+
+ // This will be instantiated implicitly but won't be imported.
+ EXCLUDE_FROM_EXPLICIT_INSTANTIATION void not_to_be_imported() noexcept;
+
+ // This won't be instantiated.
+ EXCLUDE_FROM_EXPLICIT_INSTANTIATION void not_to_be_instantiated() noexcept;
+};
+
+template <class T> void C<T>::to_be_imported() noexcept {}
+template <class T> void C<T>::not_to_be_imported() noexcept {}
+template <class T> void C<T>::not_to_be_instantiated() noexcept {}
+
+// MSC: $"?not_to_be_imported@?$C@H@@QEAAXXZ" = comdat any
+// GNU: $_ZN1CIiE18not_to_be_importedEv = comdat any
+extern template struct __declspec(dllimport) C<int>;
+
+void use() {
+ C<int> c;
+
+ // MSC: call void @"?to_be_imported@?$C@H@@QEAAXXZ"
+ // GNU: call void @_ZN1CIiE14to_be_importedEv
+ c.to_be_imported();
+
+ // MSC: call void @"?to_be_imported_explicitly@?$C@H@@QEAAXXZ"
+ // GNU: call void @_ZN1CIiE25to_be_imported_explicitlyEv
+ c.to_be_imported_explicitly(); // implicitly instantiated here
+
+ // MSC: call void @"?not_to_be_imported@?$C@H@@QEAAXXZ"
+ // GNU: call void @_ZN1CIiE18not_to_be_importedEv
+ c.not_to_be_imported(); // implicitly instantiated here
+};
+
+// MSC: declare dllimport void @"?to_be_imported@?$C@H@@QEAAXXZ"
+// GNU: declare dllimport void @_ZN1CIiE14to_be_importedEv
+
+// MSC: declare dllimport void @"?to_be_imported_explicitly@?$C@H@@QEAAXXZ"
+// GNU: declare dllimport void @_ZN1CIiE25to_be_imported_explicitlyEv
+
+// MSC: define linkonce_odr dso_local void @"?not_to_be_imported@?$C@H@@QEAAXXZ"
+// GNU: define linkonce_odr dso_local void @_ZN1CIiE18not_to_be_importedEv
|
|
@mstorsjo This fixes #135910 (comment) |
|
Thanks! If I remember correctly, there are a number of preexisting references to the problem that I'm not very familiar with the code here so I'm not sure if I can do an authoritative review - but it practically looks reasonable. Looping in @zmodem who either can do a more confident review, or find more reviewers. |
That's great! Does this, together with #168170, address the overall problem in #135910, or are there many other issues remaining in order to fix that (other than reverting my old clang change)? |
Neither of this nor #168170 is directly related to #135910 itself. For #135910 , I already have a "functional" local patch (with exporting nested classes), but I feel that I should try to find more edge cases. |
Ugh, I missed #66909. I need to add more fixes to address it. |
I think the main one is #40363 maybe. It would be good to check how this interacts with the problem described there. |
| // MSC: define linkonce_odr dso_local void @"?also_to_be_imported@?$D@H@@QEAAXXZ" | ||
| // GNU: define linkonce_odr dso_local void @_ZN1DIiE19also_to_be_importedEv |
There was a problem hiding this comment.
#40363 claims this member shouldn't be dllimport-ed but I'd oppose to that. Since dllimport is attached to the class template rather than the explicit instantiation, exclude_from_explicit_instantiation shouldn't block the inheritance of dllimport. (The current patch unintentionally aligns with that though).
There was a problem hiding this comment.
Hmm, this was a while ago so I don't have the whole context paged in, but I remember that it's tricky territory :-)
I think the problem was: how is it supposed to get exported?
Since you have an explicit instantiation decl for D<int>, you need a corresponding explicit instantiation definition on the exporting side. But if D<>::also_to_be_imported is exclude_from_explicit_instantiation, it will not get defined by that, so it doesn't get exported, and therefore cannot be imported.
rnk's comment is essentially saying that dllimport and exclude_from_explicit_instantiation have opposite purposes: dllimport says "this will be defined elsewhere", whereas exclude_from_explicit_instantiation says "this will not be explicitly defined elsewhere", and that the latter should probably win.
There was a problem hiding this comment.
Thanks for the explanation, I see the point.
However, I still feel that is confusing.
If there are no explicit instantiation declarations/definitions, should the excluded functions that are instantiated implicitly be dllimport/dllexport-ed?
If "this will not be explicitly defined elsewhere" means "therefore this will not be dllimport/dllexport-ed", then they shouldn't be, which aligns with the case of explicit instantiation.
However, people who attach the __declspec to the template rather than to an explicit instantiation would expect them to be linked across DLLs (and the compilers actually do).
That said, if the real-world cases require such behavior (i.e. not to import/export), I'm happy revising with that to solve real problems. Furthermore, if someone really wants to import/export them, attaching both of exclude_from_explicit_instantiation and __declspec should work fine.
There was a problem hiding this comment.
I agree that it's confusing.
dllimport/export on templates isn't a great idea in general, but we try to follow MSVC's behavior as best as we can. Adding exclude_from_explicit_instantiation to the mix means we're on our own, since MSVC doesn't support that.
If there are no explicit instantiation declarations/definitions, [..]
This is a fringe case, as exclude_from_explicit_instantiation is intended for use with explicit instantiations.
However, people who attach the
__declspecto the template rather than to an explicit instantiation would expect them to be linked across DLLs (and the compilers actually do).
Do people do this though? It seems very risky: how do they ensure that the instantiation on the "importing side" also exists on the "exporting side" without using explicit instantiations?
Anyway, a different way to approach this is by looking at how exclude_from_explicit_instantiation is used in practice. libc++ added it as mechanism to exclude inline (template) functions from the ABI. In that spirit it makes sense not to import/export them.
There was a problem hiding this comment.
This is a fringe case, as exclude_from_explicit_instantiation is intended for use with explicit instantiations.
exclude_from_explicit_instantiation is attached even with class templates that are not intended to be explicitly instantiated (e.g. std::tuple, behind _LIBCPP_HIDE_FROM_ABI). I'd agree that using it with __declspec on such a template is definitely quite rare.
Do people do this though? It seems very risky: how do they ensure that the instantiation on the "importing side" also exists on the "exporting side" without using explicit instantiations?
Yeah that is. But importing a whole class template rather than an explicit instantiation means exactly that. There is no way to prevent instantiation with arbitrary template parameters.
Anyway, a different way to approach this is by looking at how
exclude_from_explicit_instantiationis used in practice. libc++ added it as mechanism to exclude inline (template) functions from the ABI. In that spirit it makes sense not to import/export them.
I'll leave also_to_be_exported as not dllimport/dllexport-ed (with renaming). In the absence of explicit instantiation, they also shouldn't be imported/exported, right?
There was a problem hiding this comment.
I'll leave
also_to_be_exportedas notdllimport/dllexport-ed (with renaming).
Sounds good.
In the absence of explicit instantiation, they also shouldn't be imported/exported, right?
Maybe, (#40363 suggested extending this to non-templates too), but I think this is stretching the meaning of exclude_from_explicit_instantiation quite a bit (no explicit instantiation being involved), so maybe we should just be conservative and stick to current behavior in such cases for now.
| // GNU: define linkonce_odr dso_local void @_ZN1EIjEC2Ei | ||
|
|
||
| // MSC: declare dllimport void @"?to_be_imported@?$E@H@@UEAAXXZ" | ||
| // MSC: declare dso_local void @"?to_be_instantiated@?$E@H@@UEAAXXZ" |
There was a problem hiding this comment.
The function body should be emitted here (#66909).
kikairoya
force-pushed
the
exclude_from_explicit_instantiation-dllexport
branch
from
2c44ab5 to
73b9cf4
Compare
|
The issues are addressed. |
clang/lib/Sema/SemaDeclCXX.cpp
Outdated
| // Ensure the instance of a virtual member function which is declared | ||
| // with `__attribute__((exclude_from_explicit_instantiation))` is | ||
| // accessible from the VTable. | ||
| for (Decl *decl : Class->decls()) { | ||
| auto *Method = dyn_cast<CXXMethodDecl>(decl); | ||
| if (!Method || !Method->isVirtual()) | ||
| continue; | ||
|
|
||
| if (Method->hasAttr<ExcludeFromExplicitInstantiationAttr>()) { | ||
| MarkFunctionReferenced(Loc, Method); | ||
| DefinedAnything = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Hmm. I guess I'm confused by the contradiction of setting "DefineVTable = false" and then "ensure ... is accessible from the vtable".
The problem seems to be that some parts of the code tries to suppress vtable emission, while a different part emits it.
Would it work if we don't do the DefineVTable = false part, i.e. just changed the the if (IsExplicitInstantiationDeclaration) to something like if (IsExplicitInstantiationDeclaration && !HasExcludeFromExplicitInstantiationAttr)?
There was a problem hiding this comment.
I wasn't sure if Consumer.HandleVTable(Class); below should be called in this case; that was not called previously. However, since the VTable is actually emitted, it might need to be called.
Surprisingly, setting DefineVTable = true regardless IsExplicitInstantiationDeclaration doesn't break any tests.
The current construction was introduced in 88d292c, but the commit doesn't mention MSVC ABI.
How do I check "correct" behavior?
There was a problem hiding this comment.
I've rewritten here without changing behavior.
| struct C { | ||
| // This will be instantiated explicitly as an exported function because it | ||
| // inherits dllexport from the class instantiation. | ||
| void to_be_exported() noexcept; |
There was a problem hiding this comment.
Why is there noexcept everywhere?
There was a problem hiding this comment.
I thought a test I referenced did that but I can't remember. I'll remove them.
| // MSC: ModuleID = {{.*}}exclude_from_dllexport.cpp | ||
| // MSC: source_filename = {{.*}}exclude_from_dllexport.cpp | ||
| // GNU: ModuleID = {{.*}}exclude_from_dllexport.cpp | ||
| // GNU: source_filename = {{.*}}exclude_from_dllexport.cpp |
There was a problem hiding this comment.
These checks seem redundant. (Same in the other file.)
There was a problem hiding this comment.
It's just to silence --implicit-check=dllexport.
There was a problem hiding this comment.
I think the --implicit'check-not's are a little bit confusing here. I think the test may be easier to read with explicit checks, and then you could drop these lines.
If not, I think we need some comments explaining the check-not's.
| llvm::none_of(Class->decls(), [](Decl *decl) { | ||
| // If the class has a virtual member function declared with | ||
| // `__attribute__((exclude_from_explicit_instantiation))`, the | ||
| // explicit instantiation declaration shouldn't suppress emitting | ||
| // the vtable to ensure that the excluded member function is | ||
| // accessible through the vtable. | ||
| auto *Method = dyn_cast<CXXMethodDecl>(decl); | ||
| return Method && Method->isVirtual() && | ||
| Method->hasAttr<ExcludeFromExplicitInstantiationAttr>(); | ||
| })) |
There was a problem hiding this comment.
As a style issue, I think it would be easier to read if we put this in a variable, like:
bool HasExcludeFromExplicitInstantiation = llvm::any_of(... hasAttr<ExcludeFromExlicitInstantiationAttr>() ...
Also, I'm not sure that it matters whether the method with the attribute is virtual or not. I think it would be enough to check that any method has the attribute -- because then the motivation about "suppress the vtable; it will live with the explicit instantiation definition" may not hold.
There was a problem hiding this comment.
I wonder if we could break this out into a separate PR as well, since it seems somewhat separate from the other issue.
| // MSC: ModuleID = {{.*}}exclude_from_dllexport.cpp | ||
| // MSC: source_filename = {{.*}}exclude_from_dllexport.cpp | ||
| // GNU: ModuleID = {{.*}}exclude_from_dllexport.cpp | ||
| // GNU: source_filename = {{.*}}exclude_from_dllexport.cpp |
There was a problem hiding this comment.
I think the --implicit'check-not's are a little bit confusing here. I think the test may be easier to read with explicit checks, and then you could drop these lines.
If not, I think we need some comments explaining the check-not's.
4 participants
Attaching
__declspec(dllexport/dllimport)to explicit instantiation declaration made its whole member instantiated even if they were__attribute__((__exclude_from_explicit_instantation__)).Such members should not be instantiated nor be exported to avoid symbol leakage or duplication.
Fixes #40363
Fixes #66909